feat(clp-package): Add Python profiling decorator for internal query performance measurement. #1423
Conversation
…trument_profile
WalkthroughAdds a profiling utility using pyinstrument with a decorator for sync/async functions, extracts job/task identifiers from arguments, persists HTML/TXT profiles under CLP_LOGS_DIR, adds pyinstrument dependency, and applies the decorator to several scheduler and task entry points. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (task / scheduler)
participant Decorator as profile wrapper
participant Profiler as pyinstrument.Profiler
participant Target as Target function (sync/async)
participant FS as Filesystem
Caller->>Decorator: invoke decorated function(args, kwargs)
alt profiling enabled and no parent profiler
Decorator->>Profiler: start()
Decorator->>Target: call / await Target(args, kwargs)
Target-->>Decorator: return / raise
Decorator->>Profiler: stop()
Decorator->>FS: write HTML and TXT (section, job_id, task_id, timestamp)
Decorator-->>Caller: return / re-raise
else profiling disabled or nested
Decorator->>Target: call / await Target(args, kwargs)
Decorator-->>Caller: return / re-raise
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)components/clp-py-utils/pyproject.toml(1 hunks)components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py(2 hunks)components/job-orchestration/job_orchestration/executor/query/fs_search_task.py(2 hunks)components/job-orchestration/job_orchestration/scheduler/query/query_scheduler.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
components/job-orchestration/job_orchestration/executor/query/fs_search_task.py (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py (1)
profile(28-119)
components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py (1)
profile(28-119)
components/job-orchestration/job_orchestration/scheduler/query/query_scheduler.py (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py (1)
profile(28-119)
🪛 Ruff (0.14.0)
components/clp-py-utils/clp_py_utils/profiling_utils.py
17-17: Import from collections.abc instead: Callable
Import from collections.abc
(UP035)
17-17: typing.Tuple is deprecated, use tuple instead
(UP035)
54-54: Missing return type annotation for private function async_wrapper
(ANN202)
54-54: Missing type annotation for *args
(ANN002)
54-54: Missing type annotation for **kwargs
(ANN003)
70-71: Logging statement uses f-string
(G004)
88-88: Missing return type annotation for private function sync_wrapper
(ANN202)
88-88: Missing type annotation for *args
(ANN002)
88-88: Missing type annotation for **kwargs
(ANN003)
104-105: Logging statement uses f-string
(G004)
185-185: Do not catch blind exception: Exception
(BLE001)
186-186: Logging statement uses f-string
(G004)
216-216: Logging statement uses f-string
(G004)
222-222: datetime.datetime.now() called without a tz argument
(DTZ005)
246-246: f-string without any placeholders
Remove extraneous f prefix
(F541)
257-259: Logging statement uses f-string
(G004)
263-263: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
263-263: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
🔇 Additional comments (1)
components/clp-py-utils/pyproject.toml (1)
19-19: Verify runtime support for pyinstrument and packaging impact
- pyinstrument includes native components; confirm it installs cleanly in your worker/scheduler images and doesn’t bloat slim runtimes unnecessarily.
- If profiling is rarely enabled, consider making it an optional extra to keep minimal installs lean.
| def extract_value(param_spec: str) -> str: | ||
| """Extract value from parameter, supporting dot notation for attributes.""" | ||
| if not param_spec: | ||
| return "" | ||
|
|
||
| # Split on '.' to handle attribute access | ||
| parts = param_spec.split(".") | ||
| param_name = parts[0] | ||
|
|
||
| # Find the parameter value | ||
| value = None | ||
| if param_name in kwargs: | ||
| value = kwargs[param_name] | ||
| elif param_name in param_names: | ||
| idx = param_names.index(param_name) | ||
| if idx < len(args): | ||
| value = args[idx] | ||
|
|
||
| if value is None: | ||
| return "" | ||
|
|
||
| # Navigate through attributes if dot notation was used | ||
| for attr_name in parts[1:]: | ||
| if hasattr(value, attr_name): | ||
| value = getattr(value, attr_name) | ||
| else: | ||
| return "" | ||
|
|
||
| return str(value) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: support dict-style context extraction
If callers pass dict-like objects, walk keys as well as attributes.
Apply this diff:
- for attr_name in parts[1:]:
- if hasattr(value, attr_name):
- value = getattr(value, attr_name)
- else:
- return ""
+ for attr_name in parts[1:]:
+ if isinstance(value, dict) and attr_name in value:
+ value = value[attr_name]
+ elif hasattr(value, attr_name):
+ value = getattr(value, attr_name)
+ else:
+ return ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def extract_value(param_spec: str) -> str: | |
| """Extract value from parameter, supporting dot notation for attributes.""" | |
| if not param_spec: | |
| return "" | |
| # Split on '.' to handle attribute access | |
| parts = param_spec.split(".") | |
| param_name = parts[0] | |
| # Find the parameter value | |
| value = None | |
| if param_name in kwargs: | |
| value = kwargs[param_name] | |
| elif param_name in param_names: | |
| idx = param_names.index(param_name) | |
| if idx < len(args): | |
| value = args[idx] | |
| if value is None: | |
| return "" | |
| # Navigate through attributes if dot notation was used | |
| for attr_name in parts[1:]: | |
| if hasattr(value, attr_name): | |
| value = getattr(value, attr_name) | |
| else: | |
| return "" | |
| return str(value) | |
| def extract_value(param_spec: str) -> str: | |
| """Extract value from parameter, supporting dot notation for attributes.""" | |
| if not param_spec: | |
| return "" | |
| # Split on '.' to handle attribute access | |
| parts = param_spec.split(".") | |
| param_name = parts[0] | |
| # Find the parameter value | |
| value = None | |
| if param_name in kwargs: | |
| value = kwargs[param_name] | |
| elif param_name in param_names: | |
| idx = param_names.index(param_name) | |
| if idx < len(args): | |
| value = args[idx] | |
| if value is None: | |
| return "" | |
| # Navigate through attributes if dot notation was used | |
| - for attr_name in parts[1:]: | |
| - if hasattr(value, attr_name): | |
| - value = getattr(value, attr_name) | |
| - else: | |
| for attr_name in parts[1:]: | |
| if isinstance(value, dict) and attr_name in value: | |
| value = value[attr_name] | |
| elif hasattr(value, attr_name): | |
| value = getattr(value, attr_name) | |
| else: | |
| return "" | |
| return str(value) |
| def _is_profiling_enabled() -> bool: | ||
| """ | ||
| Check if profiling is enabled. | ||
| TODO: Add `CLPConfig` mechanism to enable/disable profiling for each component. | ||
| :return: False | ||
| """ | ||
| return False | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enable profiling via env and make interval tunable
Avoid code edits to toggle profiling. Read from env and allow interval override.
Apply this diff:
-PROFILING_INTERVAL = 0.001
+PROFILING_INTERVAL = float(os.getenv("CLP_PROFILING_INTERVAL", "0.005"))
@@
-def _is_profiling_enabled() -> bool:
+def _is_profiling_enabled() -> bool:
@@
- :return: False
+ :return: True if CLP_ENABLE_PROFILING is truthy, else False
@@
- return False
+ return os.getenv("CLP_ENABLE_PROFILING", "").lower() in {"1", "true", "yes", "on"}Also applies to: 25-25
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 191-199
(and also apply same change at line 25), replace the hardcoded return False with
logic that reads an environment variable to enable profiling (e.g.
CLP_PROFILING_ENABLED or CLP_PROFILING) and returns True for common truthy
values ("1","true","yes") and False otherwise; also make the profiling interval
tunable by reading an environment variable (e.g. CLP_PROFILING_INTERVAL_SECONDS)
elsewhere where the interval is used (or add a helper _get_profiling_interval()
that reads the env and falls back to the current default), parsing ints safely
and using a sensible default on parse failure.
| output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" | ||
| output_dir.mkdir(exist_ok=True, parents=True) | ||
|
|
There was a problem hiding this comment.
Critical: handle missing CLP_LOGS_DIR to avoid crash when enabled
Path(os.getenv("CLP_LOGS_DIR")) raises a TypeError if the env var is unset. Provide a safe fallback and warn.
Apply this diff:
- output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles"
+ logs_dir = os.getenv("CLP_LOGS_DIR")
+ if not logs_dir:
+ logger.warning("CLP_LOGS_DIR is not set; writing profiles to CWD ./profiles")
+ output_dir = Path.cwd() / "profiles"
+ else:
+ output_dir = Path(logs_dir) / "profiles"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" | |
| output_dir.mkdir(exist_ok=True, parents=True) | |
| logs_dir = os.getenv("CLP_LOGS_DIR") | |
| if not logs_dir: | |
| logger.warning("CLP_LOGS_DIR is not set; writing profiles to CWD ./profiles") | |
| output_dir = Path.cwd() / "profiles" | |
| else: | |
| output_dir = Path(logs_dir) / "profiles" | |
| output_dir.mkdir(exist_ok=True, parents=True) |
🤖 Prompt for AI Agents
components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 233-235:
currently Path(os.getenv("CLP_LOGS_DIR")) will raise a TypeError if the env var
is unset; instead read the env var into a variable, check if it's None or empty,
log a warning (use the module logger or warnings.warn) and choose a safe
fallback directory (e.g. tempfile.mkdtemp(prefix="clp_logs_") or
os.getcwd()/".clp_logs"), then construct a Path from that string and call
output_dir.mkdir(exist_ok=True, parents=True); ensure the code uses the
checked/fallback path variable rather than calling Path on a possibly None
value.
| f.write(f"CLP Query Profiling Report (pyinstrument)\n") | ||
| f.write(f"Section: {section_name}\n") | ||
| if job_id: | ||
| f.write(f"Job ID: {job_id}\n") | ||
| if task_id: | ||
| f.write(f"Task ID: {task_id}\n") | ||
| f.write(f"Timestamp: {timestamp}\n") | ||
| f.write("=" * 80 + "\n\n") | ||
| f.write(profiler.output_text(unicode=True, color=False)) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Logging/text nits: remove stray f, use logger.exception, timezone
- Remove the stray f-string with no placeholders.
- Prefer logger.exception over error(..., exc_info=True).
- Consider timezone-aware timestamps or
time.time()for filenames.
Apply this diff:
- f.write(f"CLP Query Profiling Report (pyinstrument)\n")
+ f.write("CLP Query Profiling Report (pyinstrument)\n")
@@
- logger.info(
+ logger.info(
f"Profile saved: {section_name} "
f"(duration={duration:.6f}s, samples={sample_count}) "
f"HTML={html_path}, TXT={txt_path}"
)
@@
- except Exception as e:
- logger.error(f"Failed to save profile for {section_name}: {e}", exc_info=True)
+ except Exception:
+ logger.exception("Failed to save profile for %s", section_name)Optionally:
- timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f")
+ timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y%m%d_%H%M%S_%fZ")Also applies to: 256-260, 263-263, 222-222
🧰 Tools
🪛 Ruff (0.14.0)
246-246: f-string without any placeholders
Remove extraneous f prefix
(F541)
| WorkerConfig, | ||
| ) | ||
| from clp_py_utils.clp_logging import set_logging_level | ||
| from clp_py_utils.profiling_utils import profile |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Decorator usage looks correct; add explicit context mapping and env check
- Order is right (
@profilewraps the function that Celery registers). - Consider passing
job_id_param="job_id", task_id_param="task_id"for clarity. - Ensure CLP_LOGS_DIR is set in worker env; otherwise saving profiles will fail once enabled (see _save_profile).
Also applies to: 183-184
🤖 Prompt for AI Agents
In
components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py
around lines 17 and 183-184, update the @profile decorator usage to pass
explicit context mapping by adding job_id_param="job_id" and
task_id_param="task_id" so the profiler knows which function args map to
job/task IDs, and add an environment check (e.g., verify CLP_LOGS_DIR is present
and non-empty in os.environ) before attempting to save profiles in _save_profile
to avoid failures when the worker env is missing CLP_LOGS_DIR.
| WorkerConfig, | ||
| ) | ||
| from clp_py_utils.clp_logging import set_logging_level | ||
| from clp_py_utils.profiling_utils import profile |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good instrumentation; consider explicit job/task mapping and env readiness
- Order of decorators is correct.
- Optionally pass
job_id_param="job_id", task_id_param="task_id"to be explicit. - Ensure CLP_LOGS_DIR exists in the search worker environment when profiling is enabled.
Also applies to: 169-170
🤖 Prompt for AI Agents
In
components/job-orchestration/job_orchestration/executor/query/fs_search_task.py
around line 15 (and also at lines 169-170), the profile decorator is used but
not explicit about which args map to job and task and the code assumes
CLP_LOGS_DIR exists; update the profile decorator calls to include
job_id_param="job_id", task_id_param="task_id" for clarity, and add an
environment-readiness check that ensures CLP_LOGS_DIR exists (create the
directory if missing or raise a clear error) before profiling is enabled so
profiling can write logs reliably.
| ) | ||
| from clp_py_utils.core import read_yaml_config_file | ||
| from clp_py_utils.decorators import exception_default_value | ||
| from clp_py_utils.profiling_utils import profile |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Scheduler profiling: sane targets; add guardrails for async sections and interval
- Targets are sensible;
job_id_param="job.id"is correct. - For long‑lived async functions (
acquire_reducer_for_job,handle_finished_search_job), consider:- Using a coarser interval (e.g., 5–10ms) to reduce overhead/trace size.
- Making interval and enablement configurable via env to avoid code changes when toggling.
Also applies to: 547-555, 569-611, 898-902
🤖 Prompt for AI Agents
In
components/job-orchestration/job_orchestration/scheduler/query/query_scheduler.py
around lines 49 (and also apply similar changes at 547-555, 569-611, 898-902),
the profiler is applied with defaults that are too fine-grained for long‑lived
async sections; update profiling to use a coarser sampling interval (e.g.,
5–10ms) and add guardrails: make both profiling enablement and interval
configurable via environment variables (e.g., JOB_SCHEDULER_PROFILING_ENABLED,
JOB_SCHEDULER_PROFILE_INTERVAL_MS) and wrap or conditionally apply the profiler
only when enabled so async functions like acquire_reducer_for_job and
handle_finished_search_job use the higher interval (or are skipped) to reduce
overhead and trace size.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
(not a formal review)
note we are migrating the build system from poetry to uv in #1405 so feel free to hold off on the dependency changes
| mariadb = "~1.0.11" | ||
| mysql-connector-python = "^8.2.0" | ||
| pydantic = "^2.11.9" | ||
| pyinstrument = "^5.0.0" |
There was a problem hiding this comment.
do we plan to include the profiling feature in the formal releases? if not, maybe we should put this into a
[dependency-groups]
dev = [
...
]
that said, the pyinstrument package is only ~270KB (https://pypi.org/project/pyinstrument/#files) so it shouldn't hurt to be bundled into the release.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Not a full review either. Just catch some errors when taking a glance.
It would be better if we can add some instructions in the PR to indicate:
- How to turn on the profiler (seems like to tune the flag in the code level and rebuild the package)
- A step-by-step setup for testing, and what are the expected outputs?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
components/clp-py-utils/clp_py_utils/profiling_utils.py (6)
17-17: Modernize type imports per Python 3.9+ standards.Use
collections.abc.Callableinstead oftyping.Callableand built-intupleinstead oftyping.Tuple.Based on past review comments and Ruff UP035 hints.
68-68: Make nested-profiler detection more resilient.The current substring match is case-sensitive and brittle. Consider a case-insensitive check covering broader phrasings.
Based on past review comments.
Also applies to: 100-100
185-192: Enable profiling via environment variable for better UX.Currently, profiling requires code edits to enable. Consider reading from an environment variable (e.g.,
CLP_ENABLE_PROFILING) to allow runtime control without redeployment.Apply this diff:
+import os + def _is_profiling_enabled() -> bool: """ Checks if profiling is enabled. - TODO: Add `CLPConfig` mechanism to enable/disable profiling for each component. - :return: Whether the profiler is enabled. + :return: True if CLP_ENABLE_PROFILING is truthy, else False. """ - return False + return os.getenv("CLP_ENABLE_PROFILING", "").lower() in {"1", "true", "yes", "on"}Based on past review comments.
25-25: Make profiling interval configurable.Consider reading the interval from an environment variable to allow runtime tuning without code changes.
Apply this diff:
-PROFILING_INTERVAL = 0.001 +PROFILING_INTERVAL = float(os.getenv("CLP_PROFILING_INTERVAL", "0.001"))Based on past review comments.
227-228: Critical: Handle missing CLP_LOGS_DIR to prevent crash.
Path(os.getenv("CLP_LOGS_DIR"))raisesTypeErrorif the environment variable is unset. This will crash when profiling is enabled.Apply this diff:
- output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" - output_dir.mkdir(exist_ok=True, parents=True) + logs_dir = os.getenv("CLP_LOGS_DIR") + if not logs_dir: + logger.warning("CLP_LOGS_DIR is not set; writing profiles to CWD ./profiles") + output_dir = Path.cwd() / "profiles" + else: + output_dir = Path(logs_dir) / "profiles" + output_dir.mkdir(exist_ok=True, parents=True)Based on past review comments.
240-240: Address logging and text formatting nits.Minor improvements for logging statements:
- Line 240: Remove unnecessary
fprefix from string without placeholders.- Line 257: Use
logger.exception()instead oflogger.error(..., exc_info=True).Apply this diff:
- f.write(f"CLP Query Profiling Report (pyinstrument)\n") + f.write("CLP Query Profiling Report (pyinstrument)\n") - except Exception as e: - logger.error(f"Failed to save profile for {section_name}: {e}", exc_info=True) + except Exception: + logger.exception("Failed to save profile for %s", section_name)Based on past review comments and Ruff hints (F541, G201).
Also applies to: 257-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: clp-lint
components/clp-py-utils/clp_py_utils/profiling_utils.py
[error] 97-97: Black formatting check failed. 1 file would be reformatted (profiling_utils.py). Run 'black' to format the file.
🪛 Ruff (0.14.0)
components/clp-py-utils/clp_py_utils/profiling_utils.py
17-17: Import from collections.abc instead: Callable
Import from collections.abc
(UP035)
17-17: typing.Tuple is deprecated, use tuple instead
(UP035)
54-54: Missing return type annotation for private function async_wrapper
(ANN202)
54-54: Missing type annotation for *args
(ANN002)
54-54: Missing type annotation for **kwargs
(ANN003)
70-71: Logging statement uses f-string
(G004)
86-86: Missing return type annotation for private function sync_wrapper
(ANN202)
86-86: Missing type annotation for *args
(ANN002)
86-86: Missing type annotation for **kwargs
(ANN003)
102-103: Logging statement uses f-string
(G004)
179-179: Do not catch blind exception: Exception
(BLE001)
180-180: Logging statement uses f-string
(G004)
210-210: Logging statement uses f-string
(G004)
216-216: datetime.datetime.now() called without a tz argument
(DTZ005)
240-240: f-string without any placeholders
Remove extraneous f prefix
(F541)
251-253: Logging statement uses f-string
(G004)
257-257: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
257-257: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: package-image
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (macos-15)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
components/clp-py-utils/clp_py_utils/profiling_utils.py (6)
17-17: Modernize type imports per past feedback.The past review already flagged this: import
Callablefromcollections.abcand use built-intupleinstead oftyping.Tuple.
70-71: F-string logging already flagged in past review.Past comments already noted that f-strings in logging statements should be avoided (Ruff G004). Use lazy formatting with
%or avoid f-strings in logger calls.Also applies to: 102-102
178-179: Minor code quality issues already flagged.
- Line 178: Catching blind
Exception(Ruff BLE001) - acceptable for context extraction but flagged by static analysis- Line 179: F-string in logging (Ruff G004) - already noted in past comments
184-191: Environment-based enablement already suggested in past review.A past comment already recommended reading
CLP_ENABLE_PROFILINGfrom environment to avoid code edits. The TODO indicates this will be addressed in a future PR.
226-226: CRITICAL: Missing CLP_LOGS_DIR will crash (already flagged).Past review already identified this critical issue:
Path(os.getenv("CLP_LOGS_DIR"))raisesTypeErrorif the environment variable is unset. The suggested fix provides a fallback directory and warning.
209-209: Logging and text formatting nits already flagged.Past comments already noted:
- Line 209: f-string logging (Ruff G004)
- Line 239: f-string without placeholders (Ruff F541)
- Lines 250-252: f-string logging (Ruff G004)
- Line 256: Use
logger.exceptioninstead oflogger.error(..., exc_info=True)(Ruff G201)Also applies to: 239-239, 250-252, 256-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
components/clp-py-utils/clp_py_utils/profiling_utils.py
17-17: Import from collections.abc instead: Callable
Import from collections.abc
(UP035)
17-17: typing.Tuple is deprecated, use tuple instead
(UP035)
54-54: Missing return type annotation for private function async_wrapper
(ANN202)
54-54: Missing type annotation for *args
(ANN002)
54-54: Missing type annotation for **kwargs
(ANN003)
70-71: Logging statement uses f-string
(G004)
86-86: Missing return type annotation for private function sync_wrapper
(ANN202)
86-86: Missing type annotation for *args
(ANN002)
86-86: Missing type annotation for **kwargs
(ANN003)
102-102: Logging statement uses f-string
(G004)
178-178: Do not catch blind exception: Exception
(BLE001)
179-179: Logging statement uses f-string
(G004)
209-209: Logging statement uses f-string
(G004)
215-215: datetime.datetime.now() called without a tz argument
(DTZ005)
239-239: f-string without any placeholders
Remove extraneous f prefix
(F541)
250-252: Logging statement uses f-string
(G004)
256-256: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
256-256: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: lint-check (macos-15)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-py-utils/pyproject.toml (1)
23-25: Apply formatting suggestion from previous review.A previous reviewer suggested adding a blank line after the
build-backenddeclaration for consistency.[build-system] requires = ["hatchling"] build-backend = "hatchling.build" +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/clp-py-utils/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
S3-Credentials-Design.md(1 hunks)components/clp-py-utils/pyproject.toml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
S3-Credentials-Design.md
[grammar] ~71-~71: The usual collocation for technology is “on”, not “in”.
Context: ...2. A new “Credential Manager” component in the Web UI ## Credentials management flow 1. ...
(IN_THE_INTERNET)
[uncategorized] ~93-~93: Possible missing comma found.
Context: ...ntication method doesn't require stored credentials as it uses IAM roles attached to EC2 in...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~219-~219: Using the to-infinitive may not be correct in this context. Consider using the bare infinitive (without “to”) instead.
Context: ...ss to the clp-db, letting the scheduler to access the credentials and including them in t...
(LET_IT_INFINITIVE)
[grammar] ~243-~243: The usual collocation for technology is “on”, not “in”.
Context: ...# Backend API Create new API endpoints in the web UI server: 1. GET /api/credentials -...
(IN_THE_INTERNET)
[grammar] ~252-~252: The usual collocation for technology is “on”, not “in”.
Context: ... ### Frontend UI Create a new section in the web UI for managing S3 credentials: 1. Cre...
(IN_THE_INTERNET)
[uncategorized] ~293-~293: Possible missing comma found.
Context: ...her types of credentials for future use cases such as SFTP or WebDAV authentication. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~332-~332: The usual collocation for technology is “on”, not “in”.
Context: ...ontrol (RBAC) for credential management in the web UI - Create predefined roles such a...
(IN_THE_INTERNET)
🪛 markdownlint-cli2 (0.18.1)
S3-Credentials-Design.md
44-44: Trailing spaces
Expected: 0 or 2; Actual: 4
(MD009, no-trailing-spaces)
57-57: Trailing spaces
Expected: 0 or 2; Actual: 4
(MD009, no-trailing-spaces)
59-59: Trailing spaces
Expected: 0 or 2; Actual: 4
(MD009, no-trailing-spaces)
60-60: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
82-82: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
174-174: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Hard tabs
Column: 1
(MD010, no-hard-tabs)
207-207: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
232-232: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
292-292: Trailing spaces
Expected: 0 or 2; Actual: 4
(MD009, no-trailing-spaces)
294-294: Trailing spaces
Expected: 0 or 2; Actual: 4
(MD009, no-trailing-spaces)
301-301: Trailing spaces
Expected: 0 or 2; Actual: 20
(MD009, no-trailing-spaces)
309-309: Trailing spaces
Expected: 0 or 2; Actual: 20
(MD009, no-trailing-spaces)
311-311: Trailing spaces
Expected: 0 or 2; Actual: 20
(MD009, no-trailing-spaces)
320-320: Trailing spaces
Expected: 0 or 2; Actual: 20
(MD009, no-trailing-spaces)
354-354: Bare URL used
(MD034, no-bare-urls)
367-367: Files should end with a single newline character
(MD047, single-trailing-newline)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
🔇 Additional comments (2)
components/clp-py-utils/pyproject.toml (1)
15-15: Align version constraint with previous review feedback.Past review comments suggested using the caret constraint
^5.1.1instead of>=5.1.1. The caret constraint is more conservative, preventing upgrades to potentially breaking major versions (e.g., 6.0.0+).Please confirm which version constraint is preferred and update accordingly:
- "pyinstrument>=5.1.1", + "pyinstrument^5.1.1",Additionally, ensure that
poetry lockis run incomponents/clp-py-utilsto updatepoetry.lock.S3-Credentials-Design.md (1)
1-350: Scope mismatch: This design document appears unrelated to the PR's profiling utility objectives.The PR objectives describe adding a profiling decorator utility (profiling_utils.py, applying @Profile decorators to query_scheduler.py and task functions), yet this file is a design document for AWS S3 credential management refactoring. These appear to be unrelated changes.
Please clarify whether this file should be part of this PR or if there's been a misalignment in the provided context.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
components/clp-py-utils/clp_py_utils/profiling_utils.py (8)
17-24: Modernise type hints (Callable, tuple) to satisfy Ruff UP035.Import Callable from collections.abc and use built-in tuple generics.
-from typing import Any, Callable, Tuple, TypeVar +from typing import Any, TypeVar +from collections.abc import Callable
119-126: Strengthen _extract_context_from_args types.Use precise generics and built-in tuple type; keep Callable variadic.
-def _extract_context_from_args( - func: Callable, - args: tuple, - kwargs: dict, - job_id_param: str = "job_id", - task_id_param: str = "task_id", -) -> Tuple[str, str]: +def _extract_context_from_args( + func: Callable[..., Any], + args: tuple[Any, ...], + kwargs: dict[str, Any], + job_id_param: str = "job_id", + task_id_param: str = "task_id", +) -> tuple[str, str]:
66-74: Make nested-profiler detection robust; avoid brittle string match and fix logging style.- except RuntimeError as e: - # Skip profiling this function to avoid conflicts - if "already a profiler running" in str(e): - logger.debug( - f"Skipping nested profiling for {name} " - f"(parent profiler already active)" - ) - return await func(*args, **kwargs) - raise + except RuntimeError as e: + msg = str(e).lower() + if "profiler" in msg and "already" in msg: + logger.debug( + "Skipping nested profiling for %s (parent profiler already active)", + name, + ) + return await func(*args, **kwargs) + raise @@ - except RuntimeError as e: - # Skip profiling this function to avoid conflicts - if "already a profiler running" in str(e): - logger.debug( - f"Skipping nested profiling for {name} (parent profiler already active)" - ) - return func(*args, **kwargs) - raise + except RuntimeError as e: + msg = str(e).lower() + if "profiler" in msg and "already" in msg: + logger.debug( + "Skipping nested profiling for %s (parent profiler already active)", + name, + ) + return func(*args, **kwargs) + raiseAlso applies to: 99-105
166-171: Optional: support dict-style context extraction.- for attr_name in parts[1:]: - if hasattr(value, attr_name): - value = getattr(value, attr_name) - else: - return "" + for attr_name in parts[1:]: + if isinstance(value, dict) and attr_name in value: + value = value[attr_name] + elif hasattr(value, attr_name): + value = getattr(value, attr_name) + else: + return ""
226-228: Critical: handle missing CLP_LOGS_DIR to avoid crash.Path(None) raises TypeError when env unset.
- output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" - output_dir.mkdir(exist_ok=True, parents=True) + logs_dir = os.getenv("CLP_LOGS_DIR") + if not logs_dir: + logger.warning("CLP_LOGS_DIR is not set; writing profiles to ./profiles") + output_dir = Path.cwd() / "profiles" + else: + output_dir = Path(logs_dir) / "profiles" + output_dir.mkdir(exist_ok=True, parents=True)
25-25: Make sampling interval configurable via env.Allows ops to tune without code changes.
-PROFILING_INTERVAL_SECONDS = 0.001 +PROFILING_INTERVAL_SECONDS = float(os.getenv("CLP_PROFILING_INTERVAL", "0.005"))
215-215: Use timezone‑aware timestamp (DTZ005).- timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f") + timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y%m%d_%H%M%S_%fZ")
184-191: Enable/disable profiling via env (no code edits to toggle).Reads CLP_ENABLE_PROFILING with common truthy values.
def _is_profiling_enabled() -> bool: """ - Checks if profiling is enabled. - TODO: Add `CLPConfig` mechanism to enable/disable profiling for each component. - - :return: Whether the profiler is enabled. + Checks if profiling is enabled via CLP_ENABLE_PROFILING. + :return: True if enabled, else False. """ - return False + return os.getenv("CLP_ENABLE_PROFILING", "").lower() in {"1", "true", "yes", "on"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
components/clp-py-utils/clp_py_utils/profiling_utils.py
17-17: Import from collections.abc instead: Callable
Import from collections.abc
(UP035)
17-17: typing.Tuple is deprecated, use tuple instead
(UP035)
54-54: Missing return type annotation for private function async_wrapper
(ANN202)
54-54: Missing type annotation for *args
(ANN002)
54-54: Missing type annotation for **kwargs
(ANN003)
70-71: Logging statement uses f-string
(G004)
86-86: Missing return type annotation for private function sync_wrapper
(ANN202)
86-86: Missing type annotation for *args
(ANN002)
86-86: Missing type annotation for **kwargs
(ANN003)
102-102: Logging statement uses f-string
(G004)
178-178: Do not catch blind exception: Exception
(BLE001)
179-179: Logging statement uses f-string
(G004)
209-209: Logging statement uses f-string
(G004)
215-215: datetime.datetime.now() called without a tz argument
(DTZ005)
239-239: f-string without any placeholders
Remove extraneous f prefix
(F541)
250-252: Logging statement uses f-string
(G004)
256-256: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
256-256: Logging statement uses f-string
(G004)
🔇 Additional comments (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py (1)
1-256: Exécutez Black et Ruff localement pour vérifier les déltas de formatage CI.Le fichier
components/clp-py-utils/clp_py_utilsfigure dans la portée de vérification du linting du référentiel (défini danstaskfiles/lint.yaml). Exécutez les outils de formatage localement pour identifier et corriger les déltas avant d'envoyer:black --line-length 100 components/clp-py-utils/clp_py_utils/profiling_utils.py ruff check --fix components/clp-py-utils/clp_py_utils/profiling_utils.py ruff format components/clp-py-utils/clp_py_utils/profiling_utils.py
| @functools.wraps(func) | ||
| def sync_wrapper(*args, **kwargs): | ||
| if not _is_profiling_enabled(): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add type annotations to sync wrapper (ANN002/ANN003/ANN202).
-@functools.wraps(func)
-def sync_wrapper(*args, **kwargs):
+@functools.wraps(func)
+def sync_wrapper(*args: Any, **kwargs: Any) -> Any:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @functools.wraps(func) | |
| def sync_wrapper(*args, **kwargs): | |
| if not _is_profiling_enabled(): | |
| @functools.wraps(func) | |
| def sync_wrapper(*args: Any, **kwargs: Any) -> Any: | |
| if not _is_profiling_enabled(): |
🧰 Tools
🪛 Ruff (0.14.1)
86-86: Missing return type annotation for private function sync_wrapper
(ANN202)
86-86: Missing type annotation for *args
(ANN002)
86-86: Missing type annotation for **kwargs
(ANN003)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 85-87,
the inner sync_wrapper is missing type annotations; add typing using ParamSpec
and TypeVar so the wrapper preserves the original function's parameter and
return types (e.g. declare P = ParamSpec("P") and R = TypeVar("R"), type the
wrapper as def sync_wrapper(*args: P.args, **kwargs: P.kwargs) -> R), annotate
any callable types if needed, and import ParamSpec and TypeVar from typing (or
typing_extensions for older Python); ensure the functools.wraps decorator
remains and the wrapper's return type matches the wrapped function.
| except Exception as e: | ||
| logger.debug(f"Failed to extract context from {func.__name__}: {e}") | ||
|
|
||
| return job_id, task_id |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid blind except Exception; narrow to expected errors and fix logging style.
- except Exception as e:
- logger.debug(f"Failed to extract context from {func.__name__}: {e}")
+ except (AttributeError, KeyError, IndexError, ValueError, TypeError) as e:
+ logger.debug("Failed to extract context from %s: %s", func.__name__, e)🧰 Tools
🪛 Ruff (0.14.1)
178-178: Do not catch blind exception: Exception
(BLE001)
179-179: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 178 to
181, replace the blind "except Exception" with a narrow exception handler for
the expected extraction errors (e.g., except (AttributeError, KeyError,
IndexError, ValueError) as e) and update the logging call to use structured
logging parameters and include exception info (for example logger.debug("Failed
to extract context from %s: %s", func.__name__, e, exc_info=True)) so only
relevant errors are caught and the log contains proper exception details.
| logger.debug(f"No profiling session for {section_name}") | ||
| return | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Logging hygiene: parametric logs, remove stray f, and prefer logger.exception.
- if not session:
- logger.debug(f"No profiling session for {section_name}")
+ if not session:
+ logger.debug("No profiling session for %s", section_name)
return
@@
- f.write(f"CLP Query Profiling Report (pyinstrument)\n")
+ f.write("CLP Query Profiling Report (pyinstrument)\n")
@@
- logger.info(
- f"Profile saved: {section_name} "
- f"(duration={duration:.6f}s, samples={sample_count}) "
- f"HTML={html_path}, TXT={txt_path}"
- )
+ logger.info(
+ "Profile saved: %s (duration=%.6fs, samples=%s) HTML=%s, TXT=%s",
+ section_name,
+ duration,
+ sample_count,
+ html_path,
+ txt_path,
+ )
@@
- except Exception as e:
- logger.error(f"Failed to save profile for {section_name}: {e}", exc_info=True)
+ except Exception:
+ logger.exception("Failed to save profile for %s", section_name)Also applies to: 239-246, 249-256
🧰 Tools
🪛 Ruff (0.14.1)
209-209: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 209-211
(and similarly for 239-246 and 249-256), replace f-string log calls with
parametric logging and use logger.exception where an exception context is
available; specifically, change logger.debug(f"No profiling session for
{section_name}") to logger.debug("No profiling session for %s", section_name)
(and for error cases use logger.exception("Descriptive message: %s",
section_name) inside exception handlers) and remove stray leading f characters
so all logs use structured parameters instead of f-strings.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
components/clp-py-utils/clp_py_utils/profiling_utils.py (9)
240-240: Remove extraneous f-string prefix.Line 240 has an f-string without placeholders, flagged by static analysis (F541).
Apply this diff:
- f.write(f"CLP Query Profiling Report (pyinstrument)\n") + f.write("CLP Query Profiling Report (pyinstrument)\n")Based on static analysis hints.
216-216: Use timezone-aware timestamp.
datetime.now()without atzargument produces naive timestamps, flagged by static analysis (DTZ005). Consider using UTC for consistency.Apply this diff:
- timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f") + timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y%m%d_%H%M%S_%fZ")Based on static analysis hints.
210-210: Replace f-string logging with parametric style.F-string logging is flagged by static analysis (G004).
Apply this diff:
- logger.debug(f"No profiling session for {section_name}") + logger.debug("No profiling session for %s", section_name)Based on static analysis hints and coding guidelines.
250-254: Replace f-string logging with parametric style.F-string logging is flagged by static analysis (G004).
Apply this diff:
- logger.info( - f"Profile saved: {section_name} " - f"(duration={duration:.6f}s, samples={sample_count}) " - f"HTML={html_path}, TXT={txt_path}" - ) + logger.info( + "Profile saved: %s (duration=%.6fs, samples=%s) HTML=%s, TXT=%s", + section_name, + duration, + sample_count, + html_path, + txt_path, + )Based on static analysis hints and coding guidelines.
86-87: Add type annotations to sync wrapper.The inner
sync_wrapperis missing type annotations for parameters and return type, flagged by static analysis (ANN002, ANN003, ANN202).Apply this diff:
@functools.wraps(func) -def sync_wrapper(*args, **kwargs): +def sync_wrapper(*args: Any, **kwargs: Any) -> Any:Based on static analysis hints.
256-257: Use logger.exception for exception logging.
logger.error(..., exc_info=True)should be replaced withlogger.exception(...), flagged by static analysis (G201, G004).Apply this diff:
- except Exception as e: - logger.error(f"Failed to save profile for {section_name}: {e}", exc_info=True) + except Exception: + logger.exception("Failed to save profile for %s", section_name)Based on static analysis hints and coding guidelines.
179-180: Narrow exception handling and fix logging style.The broad
except Exceptioncatches unexpected errors and masks bugs. The f-string logging is also flagged by static analysis (BLE001, G004).Apply this diff:
- except Exception as e: - logger.debug(f"Failed to extract context from {func.__name__}: {e}") + except (AttributeError, KeyError, IndexError, ValueError, TypeError) as e: + logger.debug("Failed to extract context from %s: %s", func.__name__, e)Based on static analysis hints and coding guidelines.
227-228: Critical: Handle missing CLP_LOGS_DIR to prevent TypeError.
Path(os.getenv("CLP_LOGS_DIR"))will raise aTypeErrorif the environment variable is unset (None cannot be passed to Path). This will crash when profiling is enabled.Apply this diff:
- output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" - output_dir.mkdir(exist_ok=True, parents=True) + logs_dir = os.getenv("CLP_LOGS_DIR") + if not logs_dir: + logger.warning("CLP_LOGS_DIR not set; writing profiles to ./profiles") + output_dir = Path.cwd() / "profiles" + else: + output_dir = Path(logs_dir) / "profiles" + output_dir.mkdir(exist_ok=True, parents=True)Based on past review comments.
54-55: Add type annotations to async wrapper.The inner
async_wrapperis missing type annotations for parameters and return type, flagged by static analysis (ANN002, ANN003, ANN202).Apply this diff:
@functools.wraps(func) -async def async_wrapper(*args, **kwargs): +async def async_wrapper(*args: Any, **kwargs: Any) -> Any:Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
components/clp-py-utils/clp_py_utils/profiling_utils.py
55-55: Missing return type annotation for private function async_wrapper
(ANN202)
55-55: Missing type annotation for *args
(ANN002)
55-55: Missing type annotation for **kwargs
(ANN003)
71-72: Logging statement uses f-string
(G004)
87-87: Missing return type annotation for private function sync_wrapper
(ANN202)
87-87: Missing type annotation for *args
(ANN002)
87-87: Missing type annotation for **kwargs
(ANN003)
103-103: Logging statement uses f-string
(G004)
179-179: Do not catch blind exception: Exception
(BLE001)
180-180: Logging statement uses f-string
(G004)
210-210: Logging statement uses f-string
(G004)
216-216: datetime.datetime.now() called without a tz argument
(DTZ005)
240-240: f-string without any placeholders
Remove extraneous f prefix
(F541)
251-253: Logging statement uses f-string
(G004)
257-257: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
257-257: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
|
|
||
| F = TypeVar("F", bound=Callable[..., Any]) | ||
|
|
||
| PROFILING_INTERVAL_SECONDS = 0.001 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the profiling interval configurable.
The interval is hardcoded at 0.001 seconds. Consider reading from an environment variable (e.g., CLP_PROFILING_INTERVAL_SECONDS) with a fallback to this default, allowing runtime tuning without code changes.
Based on past review comments.
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around line 26, the
profiling interval is hardcoded to 0.001s; change it to read from an environment
variable (e.g., CLP_PROFILING_INTERVAL_SECONDS) with a fallback to 0.001. Parse
the env var to a float and handle invalid values (use the default if parsing
fails or value is non-positive), and document the new variable in a comment so
runtime tuning can be done without code changes.
| logger.debug( | ||
| f"Skipping nested profiling for {name} " | ||
| f"(parent profiler already active)" | ||
| ) |
There was a problem hiding this comment.
Replace f-string logging with parametric style.
F-string logging is flagged by static analysis (G004). Use parametric logging for better performance and structured logging support.
Apply this diff for both occurrences:
- logger.debug(
- f"Skipping nested profiling for {name} "
- f"(parent profiler already active)"
- )
+ logger.debug(
+ "Skipping nested profiling for %s (parent profiler already active)",
+ name,
+ )Based on static analysis hints and coding guidelines.
Also applies to: 102-104
🧰 Tools
🪛 Ruff (0.14.1)
71-72: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 70-73
and 102-104, the logger.debug calls use f-strings which static analysis flags
(G004); change them to parametric logging by passing the message template and
variables as separate arguments (e.g. "Skipping nested profiling for %s (parent
profiler already active)", name) so the logger can handle string interpolation
and avoid evaluating the f-string eagerly; update both occurrences to use the
parametric style.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
components/clp-py-utils/clp_py_utils/profiling_utils.py (8)
179-180: Narrow exception handling and replace f-string logging.Catching blind
Exceptionis flagged by static analysis (BLE001), and f-string logging should be parametric (G004).Apply this diff:
- except Exception as e: - logger.debug(f"Failed to extract context from {func.__name__}: {e}") + except (AttributeError, KeyError, IndexError, ValueError, TypeError) as e: + logger.debug("Failed to extract context from %s: %s", func.__name__, e)Based on static analysis hints.
87-87: Add type annotations to sync wrapper.The inner
sync_wrapperlacks type annotations for parameters and return type, flagged by static analysis.Apply this diff:
@functools.wraps(func) - def sync_wrapper(*args, **kwargs): + def sync_wrapper(*args: Any, **kwargs: Any) -> Any:Based on static analysis hints.
227-228: Critical: Handle missing CLP_LOGS_DIR to avoid TypeError.
Path(os.getenv("CLP_LOGS_DIR"))raises aTypeErrorif the environment variable is unset. This will crash when profiling is enabled.Apply this diff:
- output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" + logs_dir = os.getenv("CLP_LOGS_DIR") + if not logs_dir: + logger.warning("CLP_LOGS_DIR is not set; writing profiles to CWD ./profiles") + output_dir = Path.cwd() / "profiles" + else: + output_dir = Path(logs_dir) / "profiles" output_dir.mkdir(exist_ok=True, parents=True)Based on past review comments.
26-26: Consider making the profiling interval configurable via environment variable.The interval remains hardcoded. Reading from an environment variable (e.g.,
CLP_PROFILING_INTERVAL_SECONDS) with a fallback to this default would allow runtime tuning without code changes.Apply this diff:
-PROFILING_INTERVAL_SECONDS = 0.001 +PROFILING_INTERVAL_SECONDS = float(os.getenv("CLP_PROFILING_INTERVAL_SECONDS", "0.001"))Based on past review comments.
185-192: Consider enabling profiling via environment variable as an interim solution.While the TODO mentions a future
CLPConfigmechanism, reading from an environment variable (e.g.,CLP_ENABLE_PROFILING) would allow immediate testing and deployment without code changes.Apply this diff:
def _is_profiling_enabled() -> bool: """ Checks if profiling is enabled. TODO: Add `CLPConfig` mechanism to enable/disable profiling for each component. :return: Whether the profiler is enabled. """ - return False + return os.getenv("CLP_ENABLE_PROFILING", "").lower() in {"1", "true", "yes", "on"}Based on past review comments.
210-210: Replace f-string logging with parametric style.F-string logging is flagged by static analysis (G004) and should use parametric logging.
Apply this diff:
if not session: - logger.debug(f"No profiling session for {section_name}") + logger.debug("No profiling session for %s", section_name) returnBased on static analysis hints.
69-74: Replace f-string logging with parametric style and consider resilient nested-profiler detection.F-string logging is flagged by static analysis (G004) and should use parametric logging for better performance. Additionally, the nested-profiler detection uses a case-sensitive substring match that could be more resilient.
Apply this diff:
# Skip profiling this function to avoid conflicts - if "already a profiler running" in str(e): - logger.debug( - f"Skipping nested profiling for {name} " - f"(parent profiler already active)" - ) + if "profiler" in str(e).lower() and "already" in str(e).lower(): + logger.debug( + "Skipping nested profiling for %s (parent profiler already active)", + name, + )Repeat the same change in the sync wrapper at lines 101-105.
Based on static analysis hints and past review comments.
55-55: Add type annotations to async wrapper.The inner
async_wrapperlacks type annotations for parameters and return type, flagged by static analysis.Apply this diff:
@functools.wraps(func) - async def async_wrapper(*args, **kwargs): + async def async_wrapper(*args: Any, **kwargs: Any) -> Any:Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
components/clp-py-utils/clp_py_utils/profiling_utils.py
55-55: Missing return type annotation for private function async_wrapper
(ANN202)
55-55: Missing type annotation for *args
(ANN002)
55-55: Missing type annotation for **kwargs
(ANN003)
71-72: Logging statement uses f-string
(G004)
87-87: Missing return type annotation for private function sync_wrapper
(ANN202)
87-87: Missing type annotation for *args
(ANN002)
87-87: Missing type annotation for **kwargs
(ANN003)
103-103: Logging statement uses f-string
(G004)
179-179: Do not catch blind exception: Exception
(BLE001)
180-180: Logging statement uses f-string
(G004)
210-210: Logging statement uses f-string
(G004)
216-216: datetime.datetime.now() called without a tz argument
(DTZ005)
240-240: f-string without any placeholders
Remove extraneous f prefix
(F541)
251-253: Logging statement uses f-string
(G004)
257-257: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
257-257: Logging statement uses f-string
(G004)
| args: tuple, | ||
| kwargs: dict, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use more precise type annotations for args and kwargs.
The args and kwargs parameters could use more specific generic types for better type safety.
Apply this diff:
def _extract_context_from_args(
func: Callable,
- args: tuple,
- kwargs: dict,
+ args: tuple[Any, ...],
+ kwargs: dict[str, Any],
job_id_param: str = "job_id",
task_id_param: str = "task_id",
) -> tuple[str, str]:🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 122 to
123, the params args: tuple and kwargs: dict are too generic; change them to
precise generic typing (e.g., args: Tuple[Any, ...] and kwargs: Dict[str, Any]
or Mapping[str, Any]) and add the necessary typing imports (Any, Tuple, Dict or
Mapping) at the top of the file; update any related type hints or docstrings to
match the new annotations.
| duration = session.duration | ||
| sample_count = session.sample_count | ||
|
|
||
| timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address multiple logging and formatting issues.
Several issues flagged by static analysis:
- Line 216:
datetime.now()called without timezone (DTZ005) - Line 240: Empty f-string prefix (F541)
- Lines 250-254: F-string logging (G004)
- Line 257: Should use
logger.exceptioninstead oflogger.error(..., exc_info=True)(G201)
Apply this diff:
- timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f")
+ timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y%m%d_%H%M%S_%fZ")- f.write(f"CLP Query Profiling Report (pyinstrument)\n")
+ f.write("CLP Query Profiling Report (pyinstrument)\n")- logger.info(
- f"Profile saved: {section_name} "
- f"(duration={duration:.6f}s, samples={sample_count}) "
- f"HTML={html_path}, TXT={txt_path}"
- )
+ logger.info(
+ "Profile saved: %s (duration=%.6fs, samples=%s) HTML=%s, TXT=%s",
+ section_name,
+ duration,
+ sample_count,
+ html_path,
+ txt_path,
+ )- except Exception as e:
- logger.error(f"Failed to save profile for {section_name}: {e}", exc_info=True)
+ except Exception:
+ logger.exception("Failed to save profile for %s", section_name)Based on static analysis hints.
Also applies to: 240-240, 250-257
🧰 Tools
🪛 Ruff (0.14.1)
216-216: datetime.datetime.now() called without a tz argument
(DTZ005)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 216,
240, 250-254 and 257: replace naive datetime.now() with a timezone-aware call
(e.g., datetime.datetime.now(datetime.timezone.utc) or use timezone-aware
helper) at line 216; remove the empty/incorrect f-string prefix on line 240 (use
a plain string or a proper f-string only when interpolating); change the logging
calls on lines 250-254 to use lazy interpolation (logger.debug("... %s", value)
/ logger.info("... %s", value) etc.) instead of building f-strings before
passing to the logger; and replace logger.error(..., exc_info=True) on line 257
with logger.exception(...) to log the exception with traceback.
sitaowang1998
left a comment
sitaowang1998
left a comment
There was a problem hiding this comment.
Not a formal review. Need to setup logger correctly to use the uniform format.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
components/clp-py-utils/clp_py_utils/profiling_utils.py (3)
217-217: Use timezone-aware datetime.Calling
datetime.now()without a timezone argument is flagged by static analysis (DTZ005) and was noted in past reviews.Apply this diff:
- timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f") + timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y%m%d_%H%M%S_%fZ")Based on static analysis hints and past review comments.
228-228: Critical: Handle missing CLP_LOGS_DIR to avoid crash.
Path(os.getenv("CLP_LOGS_DIR"))raises aTypeErrorif the environment variable is unset. This critical issue was flagged in past reviews.Apply this diff:
- output_dir = Path(os.getenv("CLP_LOGS_DIR")) / "profiles" + logs_dir = os.getenv("CLP_LOGS_DIR") + if not logs_dir: + logger.warning("CLP_LOGS_DIR is not set; writing profiles to CWD ./profiles") + output_dir = Path.cwd() / "profiles" + else: + output_dir = Path(logs_dir) / "profiles" output_dir.mkdir(exist_ok=True, parents=True)Based on past review comments.
180-181: Narrow exception handling and fix logging style.Catching broad
Exceptionis flagged by static analysis (BLE001) and the f-string logging violates G004. Both issues were noted in past reviews.Apply this diff:
- except Exception as e: - logger.debug(f"Failed to extract context from {func.__name__}: {e}") + except (AttributeError, KeyError, IndexError, ValueError, TypeError) as e: + logger.debug("Failed to extract context from %s: %s", func.__name__, e)Based on static analysis hints and past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/clp-py-utils/clp_py_utils/profiling_utils.py (1)
components/clp-py-utils/clp_py_utils/clp_logging.py (1)
get_logger(18-26)
🪛 Ruff (0.14.1)
components/clp-py-utils/clp_py_utils/profiling_utils.py
56-56: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
56-56: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
56-56: Dynamically typed expressions (typing.Any) are disallowed in async_wrapper
(ANN401)
72-73: Logging statement uses f-string
(G004)
88-88: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
88-88: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
88-88: Dynamically typed expressions (typing.Any) are disallowed in sync_wrapper
(ANN401)
104-104: Logging statement uses f-string
(G004)
180-180: Do not catch blind exception: Exception
(BLE001)
181-181: Logging statement uses f-string
(G004)
211-211: Logging statement uses f-string
(G004)
217-217: datetime.datetime.now() called without a tz argument
(DTZ005)
241-241: f-string without any placeholders
Remove extraneous f prefix
(F541)
252-254: Logging statement uses f-string
(G004)
258-258: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
258-258: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
| logger.debug( | ||
| f"Skipping nested profiling for {name} " | ||
| f"(parent profiler already active)" |
There was a problem hiding this comment.
Replace f-string logging with parametric style.
F-string logging is flagged by static analysis (G004) and has been noted in multiple past reviews. Use parametric logging for better performance and structured logging support.
Apply this diff for both occurrences:
- logger.debug(
- f"Skipping nested profiling for {name} "
- f"(parent profiler already active)"
- )
+ logger.debug(
+ "Skipping nested profiling for %s (parent profiler already active)",
+ name,
+ )And for line 104:
- logger.debug(
- f"Skipping nested profiling for {name} (parent profiler already active)"
- )
+ logger.debug(
+ "Skipping nested profiling for %s (parent profiler already active)",
+ name,
+ )Based on static analysis hints and past review comments.
Also applies to: 104-104
🧰 Tools
🪛 Ruff (0.14.1)
72-73: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 71-73
(and also apply to the second occurrence and line 104), replace the f-string
logger.debug calls with parametric logging: change logger.debug(f"Skipping
nested profiling for {name} (parent profiler already active)") to
logger.debug("Skipping nested profiling for %s (parent profiler already
active)", name) (and similarly update the other two occurrences) so logging uses
parameter substitution instead of f-strings.
| # Get the session for logging | ||
| session = profiler.last_session | ||
| if not session: | ||
| logger.debug(f"No profiling session for {section_name}") |
There was a problem hiding this comment.
Fix logging hygiene: remove f-strings, use logger.exception.
Multiple logging issues flagged by static analysis and past reviews:
- Line 211: f-string logging (G004)
- Line 241: empty f-string prefix (F541)
- Lines 252-254: f-string logging (G004)
- Line 258: should use
logger.exceptioninstead oflogger.error(..., exc_info=True)(G201) + f-string logging
Apply these diffs:
if not session:
- logger.debug(f"No profiling session for {section_name}")
+ logger.debug("No profiling session for %s", section_name)
return- f.write(f"CLP Query Profiling Report (pyinstrument)\n")
+ f.write("CLP Query Profiling Report (pyinstrument)\n")- logger.info(
- f"Profile saved: {section_name} "
- f"(duration={duration:.6f}s, samples={sample_count}) "
- f"HTML={html_path}, TXT={txt_path}"
- )
+ logger.info(
+ "Profile saved: %s (duration=%.6fs, samples=%s) HTML=%s, TXT=%s",
+ section_name,
+ duration,
+ sample_count,
+ html_path,
+ txt_path,
+ )- except Exception as e:
- logger.error(f"Failed to save profile for {section_name}: {e}", exc_info=True)
+ except Exception:
+ logger.exception("Failed to save profile for %s", section_name)Based on static analysis hints and past review comments.
Also applies to: 241-241, 252-254, 258-258
🧰 Tools
🪛 Ruff (0.14.1)
211-211: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/profiling_utils.py around lines 211,
241, 252-254 and 258, replace all f-string logging with logger's lazy %-style or
.format-style parameterized logging (e.g. logger.debug("No profiling session for
%s", section_name)) to avoid G004, remove the empty f-string prefix at line 241
(replace with a plain string or parameterized log), and change the error call at
line 258 that uses logger.error(..., exc_info=True) to logger.exception(...) so
the exception is logged correctly; ensure all messages pass variables as
arguments rather than interpolating them into the format string.
4 participants
Description
This pull request introduces a new lightweight profiling utility for CLP query execution based on PyInstrument and applies it to certain query execution functions. The profiling decorator generates HTML flame graphs and text summaries for performance analysis, with context extraction for job and task IDs.
Currently, profiling can only be enabled/disabled by modifying the return value of
is_profiling_enabledinprofiling_utils.py. Per-component config options should be added in a future PR.profiling_utils.pymodule inclp_py_utilsproviding aprofiledecorator for both sync and async functions, automatic context extraction, and output of HTML and text profiling reports to a designated directory.pyinstrumentas a dependency inpyproject.tomlto support profiling features.@profiledecorator to somequery_scheduler.pyfunctions.@profiledecorator tosearchandextract_streamtask functions.Profiling output is written to
<CLP_LOGS_DIR>/profilesas two files for each execution of a profiled function that are labeled with job, task, and timestamp in the filename..txtfile with call stack timing information:.htmlfile with call stack and flame graph visualization:Checklist
breaking change.
Validation performed
Performed queries using the CLP package with profiling enabled and disabled.
Summary by CodeRabbit
New Features
Chores